Skip to content

chore(charts): productionize sam-mesh chart - #302

Merged
aojea merged 26 commits into
google:mainfrom
kaisoz:kaisoz/chart-productionize
Aug 25, 2026
Merged

chore(charts): productionize sam-mesh chart#302
aojea merged 26 commits into
google:mainfrom
kaisoz:kaisoz/chart-productionize

Conversation

@kaisoz

@kaisoz kaisoz commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Adds

  • one Gateway/HTTPRoute fronting the mesh
  • bring your own OIDC issuer
  • router exposed at its node IP
  • chart hardening (restricted PSS, scoped bootstrap RBAC, probes, PVC-backed router key)
  • helm unittests
  • Dex decoupled to be used with kind

@kaisoz kaisoz changed the title chore(charts): productionize sam-mesh chart for any Gateway API cluster chore(charts): productionize sam-mesh chart Aug 25, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the sam-mesh Helm chart to support Gateway API, removes the bundled Dex in favor of external OIDC issuers, and enhances security contexts and resource limits across components. It also updates the local kind development environment to utilize cloud-provider-kind for LoadBalancer addresses instead of NodePorts. Feedback on the changes includes: avoiding hardcoded ports in the router's external address to support custom service ports; using a portable kubectl base64 decoding template in the local node script; retaining the nodePort configuration for environments where hostPort is restricted; normalizing gateway.consolePath to prevent double slashes; and allowing a custom storageClass for the router's persistent volume claim.

Comment thread charts/sam-mesh/templates/router-statefulset.yaml Outdated
Comment thread development/kind/run-local-node.sh
Comment thread charts/sam-mesh/templates/router-service.yaml
Comment thread charts/sam-mesh/templates/gateway.yaml
Comment thread charts/sam-mesh/templates/router-statefulset.yaml
@kaisoz
kaisoz force-pushed the kaisoz/chart-productionize branch from 48a1bf3 to ea0b5bb Compare August 25, 2026 11:55
@aojea

aojea commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

CI fixed in #312
Please rebase.
Apologies for the inconvenience

Comment thread .github/workflows/chart-test.yml Outdated
Comment thread charts/sam-mesh/templates/bootstrap-job.yaml Outdated
kaisoz added 24 commits August 25, 2026 15:08
router.hostPort binds 4501 TCP+UDP on the pod's node and derives the announced
multiaddrs from status.hostIP via the downward API, for TCP and QUIC both. The router
appends its own PeerID when leasing, so clients get dialable addresses.

--allow-loopback is now gated on router.allowLoopback instead of always passed, and the
Service drops its TCP-only nodePort block.

router.nodeSelector (empty by default) lets the router be pinned away from sam-node
workers: with hostPort set the router announces its own node's IP, and a sam-node
scheduled onto that same node cannot reach the hostPort in kind.
The hardcoded five-entry list came from the e2e test fixtures and was already
stale for every service under development/examples/ except calc-mcp. Expose it
as bootstrap.nodeServices, defaulting to ["*"].

Two workarounds it forced are now gone: the sam-admin binding that enumerated
per-node kind ServiceAccounts (local-node-sa never existed), and the local
allow policies in everything-mcp's node config.
One Gateway and one HTTPRoute behind gateway.enabled: the cloud's 8 exact
control-plane paths, a separately-gated dev /admin rule, and the console under
gateway.consolePath shaped exactly like the cloud deployment — the bare prefix
302s to <prefix>/ and a URLRewrite filter strips it before the console.
URLRewrite is Extended conformance, so the provider must support it.

listeners, hostnames, addresses and annotations pass through to the Gateway API
objects verbatim, so TLS, static addresses and provider annotations are all
expressible instead of hardcoded. gateway.className has no default and is
required when enabled: the class is provider-specific, so a wrong default is
worse than a clear error.
Dex is an independent component; bundling it coupled the chart to one dev
IdP's config shape. The chart now ships no identity provider:
controlPlane.oidcIssuer loses its dex default and must be set, and the kind
lane deploys its own throwaway Dex from development/kind/dex.yaml instead.
run.sh now fronts the mesh with cloud-provider-kind: the control plane and
console share one gateway address, Dex gets its own, and the router is reached
at its node IP — no NodePorts, no extraPortMappings. Dex is deployed from
dex.yaml in two passes, since its issuer and the console callback URL are the
gateway addresses that only exist once the gateways do; the console is
restarted after the final control-plane rollout because it reads the issuer
from /info once, at startup.

The local node enrolls with a bootstrap token over the gateway, the way a real
external node joins. The cluster shrinks to two sam-node workers; add labelled
workers to kind-config.yaml to host more examples at once. make kind-down
delegates to run.sh so cloud-provider-kind and its envoy containers are torn
down in dependency order.
router.externalAddrs: node containers resolve sam-router per-network via
--add-host, so the relay test's isolated network works only while the
announced multiaddr carries that name; a node-IP announcement is unroutable
from there.

bootstrap.nodeServices: policy.bats' negative path proves the control plane
denies an ungranted service, which only works while sam:role:node has a finite
list. The chart default is now ["*"], so the lane declares the grants it tests.
The kind guide now matches the gateway lane: LoadBalancer addresses from
cloud-provider-kind, the console under /console on the control plane's
address, Dex outside the chart, and a bootstrap-token local node. The
use-case docs pick up the two-node default cluster.
The unbounded mcp>=1.0.0 pulled mcp 2.0.0, which dropped mcp.server.fastmcp,
so the container crashlooped on import. calc-mcp already pins <2.0.0.
runAsNonRoot + RuntimeDefault seccomp at pod level, no privilege
escalation and all capabilities dropped per container. The curl
init/bootstrap containers pin runAsUser 100 because the image's USER is
non-numeric, which runAsNonRoot can't verify. Postgres is left alone:
its entrypoint still runs as root and steps down itself.
Each sam image gains image.tag, falling back to global.imageTag.
Pinning a new controlPlane.adminToken or db password on upgrade only
updated the Secret; the pods kept the old env values until a manual
restart. A checksum/secrets pod annotation forces the rollout. The db
statefulset is left out: postgres keeps the password initdb stored in
the PVC, so restarting it on a values change fixes nothing.
The Role allowed get/create/delete/patch/update on all secrets in the
namespace, including the admin token and db password. The job only
server-side-applies the router-token secret: get/patch are now pinned to
it via resourceNames, create stays unscoped (admission cannot match a
name on create), and delete/update are dropped.
The libp2p peer ID derives from /data/router.key; on emptyDir every
reschedule minted a new identity, forcing re-enrollment and leaving the
old peer registered until its lease expired. A volumeClaimTemplate keeps
it stable, matching .github/k8s/sam-router-template.yaml.

volumeClaimTemplates are immutable: existing installs must delete the
router StatefulSet before upgrading (the PVC is created fresh either way).
Without a readiness probe the db Service routed to postgres while initdb
or crash recovery was still running. PGDATA is deliberately not moved to
a subdirectory: existing PVCs are initialized at the volume root and a
new PGDATA would shadow their data.
…workload

The router only exposed nodeSelector; the other workloads had nothing,
so the chart could not target dedicated or tainted node pools.
They were the only containers without resources, which blocks scheduling
in namespaces whose ResourceQuota requires them. Hardcoded small values:
the containers only poll /info and POST a couple of requests.
One suite per template covering the load-bearing logic: numeric
runAsUser pins, bootstrap RBAC scoping, secret checksum rollout,
hostPort/externalAddrs address derivation, gateway routing toggles, the
router PVC and OIDC mode, scheduling passthroughs and external postgres.

Run with make helm-test; the target installs the helm-unittest plugin on
first use. Suites that assert the checksum/secrets annotation also load
templates/secrets.yaml, or the include cannot resolve.
Runs make helm-lint and make helm-test (which installs the
helm-unittest plugin on first use); helm ships on the ubuntu runners.
Path-filtered to charts/ so it stays out of unrelated PRs.
The lane reuses the sam-wi-test kind cluster across runs, so upgrading
over a release from before the router volumeClaimTemplates change fails
with 'updates to statefulset spec ... are forbidden'. On upgrade failure
delete the StatefulSets (PVCs survive) and retry. Fresh clusters, as on
hosted CI, never take the retry path.
The default RollingUpdate surges a replacement pod before freeing the old
pod's hostPort, so on nodes where every schedulable node already runs a
replica the rollout deadlocks (FailedScheduling: no free ports). Seen on
the reused e2e kind cluster: 2 replicas, 2 workers, hostPort 8080.
kaisoz added 2 commits August 25, 2026 15:08
Server-side apply falls back to update when the object exists, so patch
alone is not always enough; still pinned to the router-token secret via
resourceNames.
@kaisoz
kaisoz force-pushed the kaisoz/chart-productionize branch from cfa0b10 to 7b3c172 Compare August 25, 2026 16:03
@aojea

aojea commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the SAM mesh Helm chart and development environment to leverage Gateway API and externalize Dex, while hardening security contexts and adding comprehensive Helm unit tests. Feedback on these changes highlights a potential crash in the kind startup script if the console is disabled, a risk of invalid JSON generation in the bootstrap job if node services are null, and a recommendation to quote path values in the gateway template to prevent YAML parsing issues.

Comment thread development/kind/run.sh
Comment on lines +292 to +296
# The console discovers the issuer from the control plane's /info once, at startup, so
# restart it now that the control plane serves the Dex issuer.
echo "== Restarting the console with the final issuer =="
kubectl --context "${KCTX}" -n "${NAMESPACE}" rollout restart deployment/sam-mesh-console
kubectl --context "${KCTX}" -n "${NAMESPACE}" rollout status deployment/sam-mesh-console --timeout=180s

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If the console is disabled (e.g., via --set console.enabled=false), the sam-mesh-console deployment will not exist. Since set -e is active in this script, attempting to restart or wait for a non-existent deployment will cause the script to crash immediately. Wrap the console restart logic in a check to ensure the deployment exists before attempting to restart it.

Suggested change
# The console discovers the issuer from the control plane's /info once, at startup, so
# restart it now that the control plane serves the Dex issuer.
echo "== Restarting the console with the final issuer =="
kubectl --context "${KCTX}" -n "${NAMESPACE}" rollout restart deployment/sam-mesh-console
kubectl --context "${KCTX}" -n "${NAMESPACE}" rollout status deployment/sam-mesh-console --timeout=180s
# The console discovers the issuer from the control plane's /info once, at startup, so
# restart it now that the control plane serves the Dex issuer.
if kubectl --context "${KCTX}" -n "${NAMESPACE}" get deployment/sam-mesh-console >/dev/null 2>&1; then
echo "== Restarting the console with the final issuer =="
kubectl --context "${KCTX}" -n "${NAMESPACE}" rollout restart deployment/sam-mesh-console
kubectl --context "${KCTX}" -n "${NAMESPACE}" rollout status deployment/sam-mesh-console --timeout=180s
fi

Comment on lines +77 to +91
- matches:
- path:
type: Exact
value: {{ .Values.gateway.consolePath }}
filters:
- type: RequestRedirect
requestRedirect:
path:
type: ReplaceFullPath
replaceFullPath: {{ .Values.gateway.consolePath }}/
statusCode: 302
- matches:
- path:
type: PathPrefix
value: {{ .Values.gateway.consolePath }}/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Quoting path values in the Gateway API HTTPRoute is highly recommended to prevent YAML parsing issues if the consolePath contains special characters or is dynamically templated. Additionally, using printf with quote ensures the trailing slash is safely included within the quoted string.

  - matches:
    - path:
        type: Exact
        value: {{ .Values.gateway.consolePath | quote }}
    filters:
    - type: RequestRedirect
      requestRedirect:
        path:
          type: ReplaceFullPath
          replaceFullPath: {{ printf "%s/" .Values.gateway.consolePath | quote }}
        statusCode: 302
  - matches:
    - path:
        type: PathPrefix
        value: {{ printf "%s/" .Values.gateway.consolePath | quote }}

{"name": "sam:role:sambox", "allowed_services": ["*"], "allowed_targets": ["*"]},
{"name": "sam:role:router", "allowed_services": ["*"], "allowed_targets": ["*"]},
{"name": "sam:role:node", "allowed_services": ["mcp://calculator", "mcp://db-agent", "mcp://http-tool", "mcp://stdio-tool", "system://sam.catalog"], "allowed_targets": ["*"]}
{"name": "sam:role:node", "allowed_services": {{ toJson .Values.bootstrap.nodeServices }}, "allowed_targets": ["*"]}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If .Values.bootstrap.nodeServices is overridden to null or left empty, toJson will output null, which will result in invalid JSON inside the curl payload. Use the default function to fall back to a safe default (like list "*") to ensure the bootstrap job always renders valid JSON.

                {"name": "sam:role:node", "allowed_services": {{ toJson (default (list "*") .Values.bootstrap.nodeServices) }}, "allowed_targets": ["*"]}

@aojea
aojea merged commit 6502323 into google:main Aug 25, 2026
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants